Remove no-op failOnError argument from DeleteEntityDataFetcher by jamesfredley · Pull Request #15692 · apache/grails-core

jamesfredley · 2026-05-29T03:07:48Z

Summary

DeleteEntityDataFetcher.deleteInstance() called instance.delete(failOnError: true), but GORM's delete(Map) only honors the flush key - the failOnError argument is silently ignored. The call therefore already behaved as a plain, non-flushing delete(). The failOnError argument gave a false impression of doing something when it did nothing.

This removes that misleading argument by switching to a plain instance.delete(). Behavior is unchanged: the delete still executes without forcing a flush, exactly as before. This is a pure cleanup with no functional change - it just drops a parameter that was a no-op.

Note: an earlier revision of this PR switched the call to delete(flush: true). That would have changed behavior by forcing an immediate flush, so it was reverted in favor of the no-op delete() cleanup.

Changes

DeleteEntityDataFetcher: delete(failOnError: true) to delete().
DeleteEntityDataFetcherSpec: updated the interaction test to assert deleteInstance calls delete() with no arguments and never passes the ignored failOnError argument.

Testing

./gradlew :grails-data-graphql-core:test --tests "org.grails.gorm.graphql.fetcher.impl.DeleteEntityDataFetcherSpec" --tests "org.grails.gorm.graphql.fetcher.impl.SoftDeleteEntityDataFetcherSpec"

All pass, including the existing happy-path / invalid-id cases and the SoftDeleteEntityDataFetcher (which overrides deleteInstance) regression check.

Flagged during review of #15599.

GORM's delete(Map) only honors the flush parameter, so the failOnError: true argument passed in deleteInstance() was silently ignored. Switch to delete(flush: true) so the delete executes immediately and any failure surfaces through the data fetcher's response handler. Flagged during review of #15599. Assisted-by: claude-code:claude-4.8-opus

Copilot

Pull request overview

This pull request fixes DeleteEntityDataFetcher.deleteInstance() to use a GORM delete option that is actually honored, ensuring delete failures surface immediately and are handled by the existing try/catch response creation logic.

Changes:

Updated DeleteEntityDataFetcher.deleteInstance() from delete(failOnError: true) to delete(flush: true) so the delete is executed/flushed immediately.
Added a Spock interaction test to assert the fetcher calls delete(flush: true) and does not pass the ignored failOnError argument.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/fetcher/impl/DeleteEntityDataFetcher.groovy	Uses `flush: true` for deletes so failures surface at the delete point and can be captured by existing error handling.
grails-data-graphql/core/src/test/groovy/org/grails/gorm/graphql/fetcher/impl/DeleteEntityDataFetcherSpec.groovy	Adds an interaction test locking in the `delete(flush: true)` call and preventing regression to the ignored `failOnError` argument.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jdaugherty

I had no idea that delete doesn't honor failOnError. Seems like a gap in the API. It makes sense to remove the failOnError, but forcing a flush may have unexpected behavior. Why are you flushing by default now? Wouldn't it be better to let the transaction finish and let the transaction manager flush instead?

jdaugherty · 2026-06-02T13:13:15Z

+
+        then: 'the delete is flushed so database failures surface, and failOnError (ignored by delete) is not used'
+        1 * instance.delete([flush: true])
+        0 * instance.delete([failOnError: true])


Mock tests generally are fragile since we're not actually testing the result. Shouldn't we instead check the persisted state?

bito-code-review · 2026-06-02T13:16:13Z

The test in question uses a mock to verify that the deleteInstance method calls instance.delete with the expected flush: true parameter and avoids the deprecated failOnError: true parameter. While checking the persisted state in the database is generally more robust for integration tests, this specific test is a unit test for the DeleteEntityDataFetcher class, where mocking the GormEntity is a standard approach to verify the interaction between the fetcher and the GORM API.

grails-data-graphql/core/src/test/groovy/org/grails/gorm/graphql/fetcher/impl/DeleteEntityDataFetcherSpec.groovy

then: 'the delete is flushed so database failures surface, and failOnError (ignored by delete) is not used'
        1 * instance.delete([flush: true])
        0 * instance.delete([failOnError: true])

GORM's delete(Map) only honors the flush key, so the failOnError: true argument passed in deleteInstance() was silently ignored - the call behaved as a plain non-flushing delete(). Switching to delete(flush: true) would force an immediate flush and therefore change runtime behavior. Instead call instance.delete() with no arguments, preserving the original behavior exactly while removing the misleading failOnError argument that did nothing. Flagged during review of #15599. Assisted-by: claude-code:claude-4.8-opus

jamesfredley · 2026-06-03T19:56:56Z

Updated this PR to avoid changing any behavior.

The previous revision switched the call to instance.delete(flush: true). While that fixed the misleading failOnError argument, it also changed runtime behavior by forcing an immediate flush on every delete - which is more than this cleanup should do.

The latest commit instead changes the call to a plain instance.delete():

The original delete(failOnError: true) was effectively a non-flushing delete(), since GORM's delete(Map) only honors the flush key and silently ignores failOnError.
Plain delete() keeps that exact behavior - no flush change, no functional change.
It simply removes the failOnError parameter, which created a false impression of doing something when it did nothing.

In short: this is now a pure no-op cleanup that drops a dead parameter, not a behavior change. The interaction test was updated to assert delete() is called with no arguments and the ignored failOnError argument is never passed.

testlens-app · 2026-06-03T21:26:33Z

✅ All tests passed ✅

🏷️ Commit: 0f4fe69
▶️ Tests: 27554 executed
⚪️ Checks: 37/37 completed

Learn more about TestLens at testlens.app.

Copilot AI review requested due to automatic review settings May 29, 2026 03:07

github-project-automation Bot added this to Apache Grails May 29, 2026

Copilot started reviewing on behalf of jamesfredley May 29, 2026 03:07 View session

Copilot AI reviewed May 29, 2026

View reviewed changes

Merge branch '7.2.x' into fix/graphql-delete-failonerror

e66eebb

jdaugherty reviewed Jun 2, 2026

View reviewed changes

jamesfredley added 2 commits June 2, 2026 12:56

Merge branch '7.2.x' into fix/graphql-delete-failonerror

f02220e

jamesfredley changed the title ~~Fix ignored failOnError argument in DeleteEntityDataFetcher~~ Remove no-op failOnError argument from DeleteEntityDataFetcher Jun 3, 2026

Merge branch '7.2.x' into fix/graphql-delete-failonerror

0f4fe69

jamesfredley requested a review from jdaugherty June 3, 2026 19:58

jamesfredley self-assigned this Jun 3, 2026

jamesfredley moved this to In Progress in Apache Grails Jun 3, 2026

jamesfredley added this to the grails:7.2.0 milestone Jun 3, 2026

jdaugherty approved these changes Jun 3, 2026

View reviewed changes

jamesfredley merged commit 9d18437 into 7.2.x Jun 4, 2026
37 of 38 checks passed

github-project-automation Bot moved this from In Progress to Done in Apache Grails Jun 4, 2026

jamesfredley deleted the fix/graphql-delete-failonerror branch June 4, 2026 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove no-op failOnError argument from DeleteEntityDataFetcher#15692

Remove no-op failOnError argument from DeleteEntityDataFetcher#15692
jamesfredley merged 5 commits into
7.2.xfrom
fix/graphql-delete-failonerror

jamesfredley commented May 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

jdaugherty left a comment •

edited

Loading

Uh oh!

jdaugherty Jun 2, 2026

Uh oh!

bito-code-review Bot commented Jun 2, 2026

Uh oh!

jamesfredley commented Jun 3, 2026

Uh oh!

testlens-app Bot commented Jun 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jamesfredley commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

jdaugherty left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdaugherty Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

bito-code-review Bot commented Jun 2, 2026

Uh oh!

jamesfredley commented Jun 3, 2026

Uh oh!

testlens-app Bot commented Jun 3, 2026

✅ All tests passed ✅

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jamesfredley commented May 29, 2026 •

edited

Loading

jdaugherty left a comment •

edited

Loading